feat: add delete repository functionality#42
feat: add delete repository functionality#42Selene29 wants to merge 6 commits intorepowise-dev:mainfrom
Conversation
06c6824 to
7d71a0c
Compare
|
Full-stack delete is well done. CRUD, API, CLI, and both UI surfaces (dashboard and Danger Zone) are all covered. The FTS cleanup before CASCADE delete is the right order. Two issues I found in the code:
Logic and tests otherwise look solid. Fix the CLI argument issue and this is ready to merge. |
7d71a0c to
f781ac8
Compare
|
Took another look at this. The FTS comment addresses my concern, and re-reading the CLI args I see path is already a --option not positional, so that's fine. Just needs a rebase on main since there are conflicts now. Once that's resolved I'll merge. |
f225ae8 to
a42bab0
Compare
|
rebased |
swati510
left a comment
There was a problem hiding this comment.
Really clean PR, the ordering (collect page_ids, FTS cleanup, CASCADE delete) is exactly right and the tests cover both paths. Two things worth addressing:
-
UX bug: after deleting from /repos/[id]/settings, the web UI calls router.refresh() which re-renders the current route. That route will then 404 because the repo is gone. On the settings page the button should router.push("/") after success. From the dashboard page router.refresh() is fine.
-
FTS delete_many iterates one DELETE per page_id. For a repo with thousands of pages that's thousands of sqlite round trips. A single DELETE FROM page_fts WHERE page_id IN (...) inside the existing transaction would be much faster. Not a blocker but easy win.
Otherwise LGTM, happy to approve once the settings-page redirect is fixed.
Persistence layer for repository deletion. CASCADE on FK constraints handles child tables; FTS5 needs explicit cleanup via delete_many.
Cleans up FTS index before CASCADE-deleting the repository and all child rows (pages, graph, symbols, git metadata, decisions, etc.).
Lists repos in a numbered table, lets user pick one, confirms, then cleans FTS and CASCADE-deletes the repo. Supports --force.
Trash icon appears on hover in the repo list. Clicking shows a
confirmation dialog, then calls DELETE /api/repos/{id} and refreshes.
Reviewer feedback: - path as a second positional arg was unreachable (Click assigns left-to-right, so a path string would land in repo_id). Now --path/-p. - Added comment explaining fts is always initialized in lifespan, so the None guard is purely defensive.
- Settings page now redirects to "/" after delete instead of refreshing the now-404 route. Adds optional redirectTo prop on DeleteRepoButton. - FTS delete_many uses a single DELETE ... WHERE page_id IN (...) per chunk of 500 ids instead of one round-trip per page.
a42bab0 to
19b4f86
Compare
|
Rebased on main and addressed both points from the latest review:
|
Summary
delete_repositoryandlist_page_idsCRUD helpers, plusdelete_manyfor FTS cleanupDELETE /api/repos/{repo_id}endpoint that cascades through DB rows and FTS indexrepowise deleteCLI command with interactive repo selection and--forceflagTest plan
delete_repositoryandlist_page_idsDELETE /api/repos/{repo_id}(success + 404)repowise deleteCLI and verify repo is removed/repos/{id}/settings, verify Danger Zone section with Delete Repository button